Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

exchanges/websocket: Implement subscription configuration #1394

Merged
merged 8 commits into from
Jan 24, 2024

Conversation

gbjk
Copy link
Collaborator

@gbjk gbjk commented Nov 8, 2023

  • Move Subscriptions to it's own package
    • This allows the small type to be imported from both config and from
      stream without an import cycle, so we don't have to repeat ourselves
  • Rename Subscription.Currency to Subscription.Pair
  • Add Subscription Configuration
  • Binance support for Subscription Configuration
  • Kucoin support for Subscription Configuration

Type of change

Please delete options that are not relevant and add an x in [] as item is complete.

  • New feature (non-breaking change which adds functionality)
  • This change requires a documentation update

How has this been tested

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration and
also consider improving test coverage whilst working on a certain feature or package.

  • go test ./... -race
  • golangci-lint run

@gbjk gbjk force-pushed the feature/config_subs branch from 23ce791 to 5495fcf Compare November 8, 2023 08:15
@gloriousCode gloriousCode requested a review from a team November 8, 2023 22:15
@gloriousCode gloriousCode added the review me This pull request is ready for review label Nov 8, 2023
Copy link
Collaborator

@shazbert shazbert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a quick check, code and running looks good. 👍

exchanges/binance/binance_test.go Show resolved Hide resolved
exchanges/kucoin/kucoin_websocket.go Outdated Show resolved Hide resolved
exchanges/kucoin/kucoin_websocket.go Outdated Show resolved Hide resolved
@gbjk gbjk force-pushed the feature/config_subs branch from c53bfd5 to a1f316a Compare November 11, 2023 12:35
Copy link
Collaborator

@gloriousCode gloriousCode left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've only done a small review of the overall concept, I'm happy to see it works. A few things I've picked up and then I can get into the nitty gritty

config/README.md Show resolved Hide resolved
config/config_types.go Outdated Show resolved Hide resolved
exchanges/binance/binance_websocket.go Show resolved Hide resolved
exchanges/kucoin/kucoin_websocket.go Outdated Show resolved Hide resolved
exchanges/kucoin/kucoin_websocket.go Outdated Show resolved Hide resolved
exchanges/subscription/subscription.go Show resolved Hide resolved
gbjk added a commit to gbjk/gocryptotrader that referenced this pull request Nov 13, 2023
Replace for alias with index lookup for performance

Fixes [review comment](thrasher-corp#1394 (comment))
gbjk added a commit to gbjk/gocryptotrader that referenced this pull request Nov 13, 2023
This allows users to disable a subscription without losing definition of
it from config

Fixes [review comment](thrasher-corp#1394 (comment))
Copy link
Collaborator

@shazbert shazbert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Something that came up while testing.

exchanges/kucoin/kucoin_wrapper.go Show resolved Hide resolved
gbjk added a commit to gbjk/gocryptotrader that referenced this pull request Nov 20, 2023
@gbjk gbjk requested a review from shazbert November 20, 2023 07:02
gbjk added a commit to gbjk/gocryptotrader that referenced this pull request Nov 21, 2023
This allows users to disable a subscription without losing definition of
it from config

Fixes [review comment](thrasher-corp#1394 (comment))
gbjk added a commit to gbjk/gocryptotrader that referenced this pull request Nov 21, 2023
@gbjk gbjk force-pushed the feature/config_subs branch from eecea48 to b87ba50 Compare November 21, 2023 01:53
gbjk added a commit to gbjk/gocryptotrader that referenced this pull request Nov 21, 2023
This allows users to disable a subscription without losing definition of
it from config

Fixes [review comment](thrasher-corp#1394 (comment))
gbjk added a commit to gbjk/gocryptotrader that referenced this pull request Nov 21, 2023
@gbjk gbjk force-pushed the feature/config_subs branch from 09a5cba to 15b3258 Compare November 21, 2023 03:24
@gbjk gbjk force-pushed the feature/config_subs branch from 191a3fb to 1832ab5 Compare November 21, 2023 10:08
gbjk added a commit to gbjk/gocryptotrader that referenced this pull request Nov 22, 2023
This allows users to disable a subscription without losing definition of
it from config

Fixes [review comment](thrasher-corp#1394 (comment))
gbjk added a commit to gbjk/gocryptotrader that referenced this pull request Nov 22, 2023
@gbjk gbjk force-pushed the feature/config_subs branch 3 times, most recently from d3be869 to 64fd98c Compare November 22, 2023 05:46
@gbjk
Copy link
Collaborator Author

gbjk commented Jan 17, 2024

@thrasher- I've pulled in the abstracted form of testExch.TestInstance which was prototyped in #1358 and then built out for other exchanges.
That's allowed us to isolate the kucoin sub testing from the pair changes introduced for GetOpenInterest testing.

Also rebased to fix the master conflict.

Copy link

codecov bot commented Jan 17, 2024

Codecov Report

Attention: 342 lines in your changes are missing coverage. Please review.

Comparison is base (301551a) 43.33% compared to head (0e4eac8) 43.39%.

❗ Current head 0e4eac8 differs from pull request most recent head 44f7c91. Consider uploading reports for the commit 44f7c91 to get more accurate results

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1394      +/-   ##
==========================================
+ Coverage   43.33%   43.39%   +0.06%     
==========================================
  Files         366      367       +1     
  Lines      147087   147096       +9     
==========================================
+ Hits        63744    63837      +93     
+ Misses      75520    75436      -84     
  Partials     7823     7823              
Files Coverage Δ
currency/currencies.go 87.03% <100.00%> (+1.32%) ⬆️
exchanges/binance/binance.go 45.32% <ø> (ø)
exchanges/binance/binance_wrapper.go 38.22% <100.00%> (+0.13%) ⬆️
exchanges/bitfinex/bitfinex.go 22.90% <ø> (ø)
exchanges/kucoin/kucoin_types.go 60.86% <ø> (ø)
exchanges/kucoin/kucoin_wrapper.go 34.30% <100.00%> (+0.56%) ⬆️
exchanges/stream/websocket.go 82.55% <100.00%> (-0.20%) ⬇️
exchanges/bithumb/bithumb_websocket.go 42.85% <75.00%> (ø)
exchanges/subscription/subscription.go 93.33% <93.33%> (ø)
engine/rpcserver.go 38.37% <0.00%> (ø)
... and 28 more

... and 18 files with indirect coverage changes

@gbjk gbjk force-pushed the feature/config_subs branch from 61928be to f4eca3b Compare January 22, 2024 05:12
@gbjk
Copy link
Collaborator Author

gbjk commented Jan 22, 2024

🔁 Rebased on master

@gbjk gbjk force-pushed the feature/config_subs branch from f4eca3b to 7283b51 Compare January 22, 2024 05:15
Copy link
Collaborator

@thrasher- thrasher- left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All looking good when running live. Very cool stuff!! 🎉

Some areas of improvement for future expansion would be explicit interval support and validation, since ATM I can set wild figures in the config for Binance and it'll happily accept it. Definitely a user error thing, but something we should guard against.

image

The other thing would be that if I put one invalid subscription entry, with the rest being valid. It will correctly report the invalid subscription but no other valid subscriptions will take place and the websocket connection will be establibshed but be completely idle. So the connection will be doing nothing. We should handle this by expecting at least one valid subscription enabled (on top of the correctly reported user configured issues), otherwise disable websocket and report the issue to the user.

image

Last thing would be unification of fills feed / trade feed and subscription togglability with gctcli. Overall a very cool feature, great work!

Copy link
Collaborator

@gloriousCode gloriousCode left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have found myself confused about how your rebasing timelines work. I requested changes on Jan 2nd, but there is only changes done on November 22nd and Jan 16 which was after I approved my review on January 8th.

So do change requests get bundled before or after? It is more difficult to keep track of all pull request reviews and their state of code leaving me doubting what code specifically I had already reviewed. I'll do a final review tomorrow

edit: Shazbert's request here: #1394 (comment)
got pulled into a commit from 2 months ago, yet it was raised on January 8th, meaning I can't really trust the commit from November 22nd since it has changed since I reviewed it

internal/testing/exchange/exchange.go Show resolved Hide resolved
internal/testing/exchange/exchange.go Show resolved Hide resolved
@gbjk
Copy link
Collaborator Author

gbjk commented Jan 23, 2024

I have found myself confused about how your rebasing timelines work. I requested changes on Jan 2nd, but there is only changes done on November 22nd and Jan 16 which was after I approved my review on January 8th.

So do change requests get bundled before or after? It is more difficult to keep track of all pull request reviews and their state of code leaving me doubting what code specifically I had already reviewed. I'll do a final review tomorrow

Rebased after you approved and added gcrc. At that point all changes you'd asked for and subsequently approved get fixuped down to their respective original commits.

Then any commits added after that are left for you to see again.

Specifically the code you haven't reviewed was 94da21f and 7283b51.

edit: Shazbert's request here: #1394 (comment) got pulled into a commit from 2 months ago, yet it was raised on January 8th, meaning I can't really trust the commit from November 22nd since it has changed since I reviewed it

Yes. The commit was fixed on 8th January and merged down on 15th January ( specifically after you signalled you were done ).

The commits change, but the body of code is as you last saw it. The autosquash just moves the commits down and merges them once you've approved your changes.

As discussed previously, I'm leaving everything completely alone until each member is done, and then I merge it at that point.

Since it seems confusing, I can leave it right until the end and then rebase at the very last moment?
Normally fixups get merged when you rebase on master, but I can avoid that.

@gloriousCode gloriousCode removed the gcrc GloriousCode Review Complete label Jan 24, 2024
Copy link
Collaborator

@gloriousCode gloriousCode left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your explanations. I can put my reply which I messaged you here if you'd like.
I re-reviewed everything and only found the tiniest thing 😄

exchanges/binance/binance_test.go Outdated Show resolved Hide resolved
@thrasher- thrasher- changed the title Subscription Configuration exchanges/websocket: Implement subscription configuration Jan 24, 2024
@gbjk gbjk requested a review from gloriousCode January 24, 2024 02:30
@gbjk
Copy link
Collaborator Author

gbjk commented Jan 24, 2024

Thank you for your explanations. I can put my reply which I messaged you here if you'd like. I re-reviewed everything and only found the tiniest thing 😄

Thank you 🙏 Ready again.

Copy link
Collaborator

@gloriousCode gloriousCode left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tACK! Thanks for making those changes!

gbjk added 8 commits January 24, 2024 10:18
This allows the small type to be imported from both `config` and from
`stream` without an import cycle, so we don't have to repeat ourselves
This was being mis-used through much of the code, and since we're
already touching everything, we might as well fix it
* Simplify GenerateDefaultSubs
* Improve TestGenSubs coverage
* Test Candle Sub generation
* Support Candle intervals
* Full responsibility for formatting Channel name on GenerateDefaultSubs
  OR consumer of Subscribe
* Simplify generatePayloads as a result
* Fix test coverage of asset types in processMarketSnapshot
Use isolated test instance for `TestGetOpenInterest`.

`TestGetOpenInterest` would occassionally change pairs before
GenerateDefault Subs.
@gbjk gbjk force-pushed the feature/config_subs branch from 0e4eac8 to 44f7c91 Compare January 24, 2024 03:26
Copy link
Collaborator

@thrasher- thrasher- left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great stuff @gbjk, awesome feature!

@thrasher- thrasher- merged commit e007f69 into thrasher-corp:master Jan 24, 2024
4 of 10 checks passed
@gbjk gbjk deleted the feature/config_subs branch January 24, 2024 05:03
gbjk added a commit to gbjk/gocryptotrader that referenced this pull request May 16, 2024
gbjk added a commit to gbjk/gocryptotrader that referenced this pull request May 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
review me This pull request is ready for review szrc shazbert review complete
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants